fix rounding issue with exponents in fmt #116301

mj10021 · 2023-09-30T23:02:09Z

fixes issue #115737 , where the decimal places are rounded incorrectly when formatting scientific notation

rustbot · 2023-09-30T23:02:16Z

(rustbot has picked a reviewer for you, use r? to override)

cuviper · 2023-09-30T23:43:05Z

library/core/tests/fmt/num.rs

+    assert_eq!(format!("{:.2e}",  1925), "1.93e3" );
+    assert_eq!(format!("{:.2e}",  1945), "1.95e3" );
+    assert_eq!(format!("{:.2e}",  1965), "1.97e3" );
+    assert_eq!(format!("{:.2e}",  1985), "1.99e3" );


Aren't we supposed to be rounding ties toward even? So this would be "1.98e3", and many others would round down too.

Is round to even spec'd in a doc? I was trying to keep the same behavior as before using https://doc.rust-lang.org/std/fmt/#precision as a reference for desired behavior based on the discussion here: https://users.rust-lang.org/t/scientific-notation-decimal-places/99682/5

Sort of -- it's specified for operations on the f32 page, and the f64 page just references that.

When the number resulting from a primitive operation (addition, subtraction, multiplication, or division) on this type is not exactly representable as f32, it is rounded according to the roundTiesToEven direction defined in IEEE 754-2008.

We do seem to do this for fractional formatting as well:

fn main() { println!("{:.2e}", 1.625); println!("{:.2e}", 16.25); println!("{:.2e}", 162.5); println!("{:.2e}", 1625); println!("{:.2e}", 16250); }

Note that my choice of input values are represented precisely at all levels, so there's no ambiguity that these should be ties. The first three do round down to even, but the last two are rounding up.

1.62e0 1.62e1 1.62e2 1.63e3 1.63e4

(although I tested that on the playground, not your code.)

Ah, I see what you're saying. Since the bug submitted was the number of digits expressed, would it be ok for me to take out the assert!s that enforce the non round-to-even behavior and then work on the rounding in a follow up PR?

If you're willing to work on both, I'd rather do it all at once so they can land in the same release, and we only have to explain changes once. :)

gotcha 🙂 i think what I just pushed rounds to evens on a tie correctly now

cuviper · 2023-11-09T23:38:49Z

library/core/src/fmt/num.rs

@@ -330,10 +330,17 @@ macro_rules! impl_Exp {
                if subtracted_precision != 0 {
                    let rem = n % 10;
                    n /= 10;
+                    let last_digit_odd = (n % 10) % 2 != 0;


You don't need to isolate the last digit to know that it's odd.

Suggested change

let last_digit_odd = (n % 10) % 2 != 0;

let last_digit_odd = n % 2 != 0;

cuviper · 2023-11-09T23:53:41Z

library/core/src/fmt/num.rs

                        n += 1;
+                        // if the digit is rounded to the next power
+                        // instead adjust the exponent
+                        if n % 10 == 0 {


Hmm, doesn't this only tell you that you overflowed the least significant digit? e.g. maybe n=129 rolled up to 130, but that doesn't need any adjustment. For a new power of 10, like n=99 to 100, maybe something like n.log10() > (n - 1).log10() would work?

cuviper · 2023-11-10T17:07:17Z

library/core/src/fmt/num.rs

-                    // round up last digit
-                    if rem >= 5 {
+                    // round up last digit, round to even on a tie
+                    if rem > 5 || (rem == 5 && n % 2 != 0) {


Sorry I keep noticing things, but I think we're almost there...

The check for even here is missing whether we removed any additional non-zero digits earlier. e.g., for {:.1e} I expect:

124999 => 1.2e5 (rounded down)

125000 => 1.2e5 (tie rounded down to even)

125001 => 1.3e5 (rounded up)

This one gets treated like a tie right now and prints 1.2e5

134999 1.3e5 (rounded down)

135000 1.4e5 (tie rounded up to even)

135001 1.4e5 (rounded up)

I think subtracted_precision > 1 would account for the extra non-zero digits? Please do add these values as tests.

cuviper · 2023-11-10T17:12:18Z

library/core/tests/fmt/num.rs

+    assert_eq!(format!("{:.2e}",  1925), "1.92e3" );
+    assert_eq!(format!("{:.2e}",  1945), "1.94e3" );
+    assert_eq!(format!("{:.2e}",  1965), "1.96e3" );
+    assert_eq!(format!("{:.2e}",  1985), "1.98e3" );


The more I look at this giant list of tests, the less I'm sure of its value. There are certainly some edge cases in here that we want to keep, but many are duplicating the exact same conditions. This makes it hard to see whether we actually have a test corresponding to the different pieces of the implementation.

For example, is there a test that exercises the power-of-10 overflow that we checked last time? I'm not sure.

Can we narrow it down to fewer targeted tests around different boundary conditions?

Another possibility could be an exhaustive test comparing to the independent floating point implementation. Like, check all i16 values compared to the same f32 value, formatted to a few different levels of precision.

for i in i16::MIN..=i16::MAX { for p in 0..=5 { assert_eq!( format!("{i:.p$e}"), format!("{:.p$e}", f32::from(i)), "integer {i} at precision {p}" ); } }

cuviper · 2023-11-13T19:55:17Z

LGTM, thanks!

@bors r+
@rustbot label +relnotes

bors · 2023-11-13T19:55:19Z

📌 Commit 3f0908f has been approved by cuviper

It is now in the queue for this repository.

bors · 2023-11-14T00:04:08Z

⌛ Testing commit 3f0908f with merge b917524...

bors · 2023-11-14T01:59:51Z

☀️ Test successful - checks-actions
Approved by: cuviper
Pushing b917524 to master...

rust-timer · 2023-11-14T03:22:14Z

Finished benchmarking commit (b917524): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.8%	[0.8%, 0.8%]	1
Regressions ❌ (secondary)	3.2%	[0.8%, 6.0%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.0%	[-2.0%, -2.0%]	1
All ❌✅ (primary)	0.8%	[0.8%, 0.8%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-0.7%, -0.7%]	1
Improvements ✅ (secondary)	-3.1%	[-3.1%, -3.1%]	1
All ❌✅ (primary)	-0.7%	[-0.7%, -0.7%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 674.708s -> 674.219s (-0.07%)
Artifact size: 311.11 MiB -> 311.04 MiB (-0.02%)

…-Simulacrum avoid exhaustive i16 test in Miri rust-lang#116301 added a test that is way too slow to be running in Miri. So let's only test a few hopefully representative cases.

Rollup merge of rust-lang#117946 - RalfJung:miri-libcore-test, r=Mark-Simulacrum avoid exhaustive i16 test in Miri rust-lang#116301 added a test that is way too slow to be running in Miri. So let's only test a few hopefully representative cases.

Pkgsrc changes: * Adapt checksums and patches. Upstream chnages: Version 1.76.0 (2024-02-08) ========================== Language -------- - [Document Rust ABI compatibility between various types] (rust-lang/rust#115476) - [Also: guarantee that char and u32 are ABI-compatible] (rust-lang/rust#118032) - [Warn against ambiguous wide pointer comparisons] (rust-lang/rust#117758) Compiler -------- - [Lint pinned `#[must_use]` pointers (in particular, `Box<T>` where `T` is `#[must_use]`) in `unused_must_use`.] (rust-lang/rust#118054) - [Soundness fix: fix computing the offset of an unsized field in a packed struct] (rust-lang/rust#118540) - [Soundness fix: fix dynamic size/align computation logic for packed types with dyn Trait tail] (rust-lang/rust#118538) - [Add `$message_type` field to distinguish json diagnostic outputs] (rust-lang/rust#115691) - [Enable Rust to use the EHCont security feature of Windows] (rust-lang/rust#118013) - [Add tier 3 {x86_64,i686}-win7-windows-msvc targets] (rust-lang/rust#118150) - [Add tier 3 aarch64-apple-watchos target] (rust-lang/rust#119074) - [Add tier 3 arm64e-apple-ios & arm64e-apple-darwin targets] (rust-lang/rust#115526) Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries --------- - [Add a column number to `dbg!()`] (rust-lang/rust#114962) - [Add `std::hash::{DefaultHasher, RandomState}` exports] (rust-lang/rust#115694) - [Fix rounding issue with exponents in fmt] (rust-lang/rust#116301) - [Add T: ?Sized to `RwLockReadGuard` and `RwLockWriteGuard`'s Debug impls.] (rust-lang/rust#117138) - [Windows: Allow `File::create` to work on hidden files] (rust-lang/rust#116438) Stabilized APIs --------------- - [`Arc::unwrap_or_clone`] (https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#method.unwrap_or_clone) - [`Rc::unwrap_or_clone`] (https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#method.unwrap_or_clone) - [`Result::inspect`] (https://doc.rust-lang.org/stable/std/result/enum.Result.html#method.inspect) - [`Result::inspect_err`] (https://doc.rust-lang.org/stable/std/result/enum.Result.html#method.inspect_err) - [`Option::inspect`] (https://doc.rust-lang.org/stable/std/option/enum.Option.html#method.inspect) - [`type_name_of_val`] (https://doc.rust-lang.org/stable/std/any/fn.type_name_of_val.html) - [`std::hash::{DefaultHasher, RandomState}`] (https://doc.rust-lang.org/stable/std/hash/index.html#structs) These were previously available only through `std::collections::hash_map`. - [`ptr::{from_ref, from_mut}`] (https://doc.rust-lang.org/stable/std/ptr/fn.from_ref.html) - [`ptr::addr_eq`](https://doc.rust-lang.org/stable/std/ptr/fn.addr_eq.html) Cargo ----- See [Cargo release notes] (https://github.com/rust-lang/cargo/blob/master/CHANGELOG.md#cargo-176-2024-02-08). Rustdoc ------- - [Don't merge cfg and doc(cfg) attributes for re-exports] (rust-lang/rust#113091) - [rustdoc: allow resizing the sidebar / hiding the top bar] (rust-lang/rust#115660) - [rustdoc-search: add support for traits and associated types] (rust-lang/rust#116085) - [rustdoc: Add highlighting for comments in items declaration] (rust-lang/rust#117869) Compatibility Notes ------------------- - [Add allow-by-default lint for unit bindings] (rust-lang/rust#112380) This is expected to be upgraded to a warning by default in a future Rust release. Some macros emit bindings with type `()` with user-provided spans, which means that this lint will warn for user code. - [Remove x86_64-sun-solaris target.] (rust-lang/rust#118091) - [Remove asmjs-unknown-emscripten target] (rust-lang/rust#117338) - [Report errors in jobserver inherited through environment variables] (rust-lang/rust#113730) This [may warn](rust-lang/rust#120515) on benign problems too. - [Update the minimum external LLVM to 16.] (rust-lang/rust#117947) - [Improve `print_tts`](rust-lang/rust#114571) This change can break some naive manual parsing of token trees in proc macro code which expect a particular structure after `.to_string()`, rather than just arbitrary Rust code. - [Make `IMPLIED_BOUNDS_ENTAILMENT` into a hard error from a lint] (rust-lang/rust#117984) - [Vec's allocation behavior was changed when collecting some iterators] (rust-lang/rust#110353) Allocation behavior is currently not specified, nevertheless changes can be surprising. See [`impl FromIterator for Vec`] (https://doc.rust-lang.org/nightly/std/vec/struct.Vec.html#impl-FromIterator%3CT%3E-for-Vec%3CT%3E) for more details. - [Properly reject `default` on free const items] (rust-lang/rust#117818)

rustbot assigned cuviper Sep 30, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 30, 2023

cuviper reviewed Sep 30, 2023

View reviewed changes

mj10021 force-pushed the issue-115737-fix branch from 94ab4d2 to 1341e63 Compare October 27, 2023 23:09

cuviper reviewed Nov 9, 2023

View reviewed changes

mj10021 force-pushed the issue-115737-fix branch 2 times, most recently from 0647635 to 4e4c972 Compare November 10, 2023 02:54

cuviper reviewed Nov 10, 2023

View reviewed changes

mj10021 added 2 commits November 11, 2023 16:40

fix rounding issue with exponents in fmt

e81964e

round to even

3f0908f

mj10021 force-pushed the issue-115737-fix branch from 4e4c972 to 3f0908f Compare November 11, 2023 22:31

rustbot added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 13, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 13, 2023

cuviper linked an issue Nov 13, 2023 that may be closed by this pull request

scientific notation incorrect decimal places #115737

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 14, 2023

bors merged commit b917524 into rust-lang:master Nov 14, 2023
12 checks passed

rustbot added this to the 1.76.0 milestone Nov 14, 2023

RalfJung mentioned this pull request Nov 15, 2023

avoid exhaustive i16 test in Miri #117946

Merged

mj10021 deleted the issue-115737-fix branch November 1, 2024 22:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix rounding issue with exponents in fmt #116301

fix rounding issue with exponents in fmt #116301

mj10021 commented Sep 30, 2023

rustbot commented Sep 30, 2023

cuviper Sep 30, 2023

mj10021 Sep 30, 2023

cuviper Oct 1, 2023 •

edited

Loading

mj10021 Oct 1, 2023

cuviper Oct 18, 2023

mj10021 Oct 27, 2023

cuviper Nov 9, 2023

cuviper Nov 9, 2023

cuviper Nov 10, 2023

cuviper Nov 10, 2023

cuviper Nov 10, 2023

cuviper Nov 10, 2023

cuviper commented Nov 13, 2023

bors commented Nov 13, 2023

bors commented Nov 14, 2023

bors commented Nov 14, 2023

rust-timer commented Nov 14, 2023

	let last_digit_odd = (n % 10) % 2 != 0;
	let last_digit_odd = n % 2 != 0;

fix rounding issue with exponents in fmt #116301

fix rounding issue with exponents in fmt #116301

Conversation

mj10021 commented Sep 30, 2023

rustbot commented Sep 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuviper Oct 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuviper commented Nov 13, 2023

bors commented Nov 13, 2023

bors commented Nov 14, 2023

bors commented Nov 14, 2023

rust-timer commented Nov 14, 2023

Overall result: no relevant changes - no action needed

cuviper Oct 1, 2023 •

edited

Loading